Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for links of file paths with spaces (#13245) #43733

Merged

Conversation

doivosevic
Copy link

Hi.
This is just a regex improvement to capture links with spaces as well.
The approach is to use an OR non capturing group which does (validChar | space+validChar)* instead of previously using just validChar*

Fixes:
#13245
and
microsoft/AL#167

@doivosevic
Copy link
Author

doivosevic commented Feb 15, 2018

I see that I have 1 failing:

  1. workspace-namespace findFiles:
    AssertionError: 0 == 1

    • expected - actual
      -0
      +1

    at C:\projects\vscode\extensions\vscode-api-tests\src\singlefolder-tests\workspace.test.ts:499:11

Is this a know instability or is it using this link pattern matching?
For some reason I cannot debug or run this test. When I do

PS C:\git\vscode> .\scripts\test.bat --run "C:/git/vscode/extensions/vscode-api-tests/out/singlefolder-tests/workspace.test.js"

it hangs and doesn't do anything.

Also, I see that 1 of the 3 travis jobs failed after 50 mins for hanging.
Could these issues be related to my changes?

@bpasero bpasero assigned isidorn and unassigned bpasero Feb 15, 2018
@isidorn isidorn added this to the February 2018 milestone Feb 16, 2018
@isidorn
Copy link
Contributor

isidorn commented Feb 16, 2018

@DominikDitoIvosevic thanks for your PR.
Great that you have written tests. On top of that you could update the comment above the regex since it is quite complex so to make it easier for understanding.

Travis failing you can ignore. However unit tests you probably broke since they are passing nicely wihtout your changes.
Please look into fixing them.
You can run the unit tests by using the launch configuration "Unit Tests"
Here's our contributing document which should have more info on this subject https://github.com/Microsoft/vscode/wiki/How-to-Contribute

…into bugs/13245/doivosev/linkSpaceCaptureFix
@doivosevic
Copy link
Author

@isidorn I was locally having issues with running tests because I was using the default git checkout style using clrf and your tests are hardcoded to work with LF. I see that after remerging with master my tests pass on appveyor but travis still fails. Does this mean the last fail was an instability and my PR is ok?

@isidorn isidorn modified the milestones: February 2018, March 2018 Feb 22, 2018
@doivosevic
Copy link
Author

I tried allowing spaces in all the file patterns but there seems to be a bigger issue here. You have to choose between more path characters or more variations from which we can extract paths.

One problem is that while on Windows <>" etc are invalid path characters they are valid on linux. Also, one test wasn't catching ] and ' which are valid on all operating systems so I suppose that was a mistake. The reason why the last of the 3 patterns doesn't allow spaces in the path is because you allow for not specifying neither the line nor the character for that pattern and since space is a valid character in the file extension it's not possible to match your current tests.

While I think it's important to support spaces for all path patterns, allowing spaces for the last of the 3 patterns would require you to enforce specifying some character after the path. For instance some tests have in after the path but none of the patterns require the in to be there. We could split the last pattern to allow for neither the line or the char but to require in after the path. That decision is up to you.

@doivosevic
Copy link
Author

@isidorn could you please tell me if there is something else I need to do? Could we get a review started up?
I've split the patterns into template literals to hopefully make it more clear what's going on.

@isidorn
Copy link
Contributor

isidorn commented Feb 26, 2018

@DominikDitoIvosevic Hi, this week we are in the endgame week, which means we only take limited code changes we are cruical for the release.
Due to that I will review this PR next week in our debt week. Than I will provide feedback. Thanks

@doivosevic
Copy link
Author

@isidorn Does this mean this fix is potentially getting into 1.22 or 1.23?

@isidorn
Copy link
Contributor

isidorn commented Mar 5, 2018

@DominikDitoIvosevic 1.22

@isidorn
Copy link
Contributor

isidorn commented Mar 6, 2018

@DominikDitoIvosevic I checked out your changes and they make sense to me. I would merge them in so we try them out in insiders and see if something breaks.
However also adding @alexandrudima since he is the original author looking at commits to see if he objects about some changes here.

@DominikDitoIvosevic did you bild vscode with your changes and verify nothing gets badly broken? Like detecting links in various outputs?

@alexdima
Copy link
Member

alexdima commented Mar 7, 2018

@isidorn Adding @bpasero. He is the original author of the output link computer.

@alexdima alexdima assigned bpasero and unassigned alexdima Mar 7, 2018
@isidorn
Copy link
Contributor

isidorn commented Mar 7, 2018

Let's merge this in and see how it behaves in the wild.

@isidorn isidorn merged commit d4c5873 into microsoft:master Mar 7, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants